Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces a new @ArrayContains decorator, including its implementation, documentation, and examples. However, a critical vulnerability has been identified in its core validation logic. The implementation is inverted, causing it to fail for all arrays and potentially crash the server with non-iterable inputs, posing a minor stability risk (DoS via 500 errors). This bug renders the decorator unusable and requires immediate correction.
|
/gemini review |
There was a problem hiding this comment.
Code Review
새로운 @ArrayContains 데코레이터 추가와 관련 문서 및 예제 작성을 포함한 기여에 감사드립니다. 전반적으로 좋은 구현이지만, 몇 가지 개선점을 제안합니다. isDeepEqual 유틸리티 함수에서 순환 참조를 처리하지 못해 무한 루프에 빠질 수 있는 심각한 문제가 발견되었습니다. 또한, 예제 코드의 @Type 데코레이터에서 null 값을 제대로 처리하지 못하는 잠재적 버그가 있으며, 스타일 가이드에 따라 일부 주석을 한국어로 변경할 것을 제안합니다. 자세한 내용은 각 파일의 주석을 참고해주세요.
| export function isDeepEqual(obj1: any, obj2: any): boolean { | ||
| if (obj1 === obj2) return true | ||
|
|
||
| const stack: [any, any][] = [[obj1, obj2]] | ||
| while (stack.length > 0) { | ||
| const [a, b] = stack.pop()! | ||
|
|
||
| if (a === b) continue | ||
| if (typeof a !== 'object' || a === null || typeof b !== 'object' || b === null) { | ||
| return false | ||
| } | ||
|
|
||
| // Handle Date objects specifically | ||
| if (a instanceof Date && b instanceof Date) { | ||
| if (a.getTime() !== b.getTime()) return false | ||
| continue | ||
| } | ||
|
|
||
| // Check if both are arrays or neither (for accuracy and performance) | ||
| const isArrayA = Array.isArray(a) | ||
| const isArrayB = Array.isArray(b) | ||
| if (isArrayA !== isArrayB) return false | ||
|
|
||
| const keysA = Object.keys(a) | ||
| const keysB = Object.keys(b) | ||
| if (keysA.length !== keysB.length) return false | ||
|
|
||
| for (const key of keysA) { | ||
| if (!Object.prototype.hasOwnProperty.call(b, key)) return false | ||
| stack.push([a[key], b[key]]) | ||
| } | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
isDeepEqual 함수는 순환 참조(circular reference)가 있는 객체를 처리할 때 무한 루프에 빠질 수 있는 취약점이 있습니다. 이는 서비스 거부(DoS) 공격으로 이어질 수 있습니다. 예를 들어, const a = {}; a.a = a;와 같은 객체가 입력으로 들어오면 스택에 동일한 비교 쌍이 계속 추가되어 무한 루프가 발생합니다.
이 문제를 해결하기 위해 이미 비교한 객체 쌍을 추적하는 메커니즘을 추가해야 합니다. Map과 Set을 사용하여 방문한 객체 쌍을 기록하는 방식으로 수정하는 것을 제안합니다.
export function isDeepEqual(obj1: any, obj2: any): boolean {
if (obj1 === obj2) return true
// 순환 참조를 처리하기 위해 방문한 객체 쌍을 추적합니다.
const visited = new Map<object, Set<object>>()
const stack: [any, any][] = [[obj1, obj2]]
while (stack.length > 0) {
const [a, b] = stack.pop()!
if (a === b) continue
if (typeof a !== 'object' || a === null || typeof b !== 'object' || b === null) {
return false
}
// 이미 비교한 쌍이면 건너뜁니다.
if (visited.has(a) && visited.get(a)!.has(b)) {
continue
}
if (!visited.has(a)) {
visited.set(a, new Set())
}
visited.get(a)!.add(b)
// Date 객체를 특별히 처리합니다.
if (a instanceof Date && b instanceof Date) {
if (a.getTime() !== b.getTime()) return false
continue
}
// 두 값 모두 배열이거나 아니어야 합니다.
const isArrayA = Array.isArray(a)
const isArrayB = Array.isArray(b)
if (isArrayA !== isArrayB) return false
const keysA = Object.keys(a)
const keysB = Object.keys(b)
if (keysA.length !== keysB.length) return false
for (const key of keysA) {
if (!Object.prototype.hasOwnProperty.call(b, key)) return false
stack.push([a[key], b[key]])
}
}
return true
}There was a problem hiding this comment.
HTTP 요청의 JSON 데이터는 직렬화된 문자열 형태로 전송되며, 표준 JSON 규격상 순환 참조를 포함할 수 없습니다. 또한, 우리 라이브러리 이전 단계인 express.json()에서 JSON.parse()를 통해 객체화되는데, 이 과정에서 순환 구조가 있다면 이미 파싱 에러가 발생합니다.
그러므로 발생할 수 없는 케이스를 위해 모든 객체 노드를 순회하며 순환 참조를 체크하는 로직을 추가하는 것은 라이브러리의 성능에 불필요한 비용을 발생시킨다고 생각하여 해당 리뷰는 반영하지 않도록 하겠습니다.
| * @param values - The values that must be present in the array. | ||
| * @param message - Optional custom error message. | ||
| */ | ||
| export function ArrayContains(values: any[], message?: cargoErrorMessage): TypedPropertyDecorator<any[]> { |
There was a problem hiding this comment.
equal 여부 판단을 위한 함수도 optional 로 개발자가 입력할수 있게 하면 어떨가요?
There was a problem hiding this comment.
기본적으로 저희가 제공하는 함수를 사용하되, 추가적인 옵션을 받자는 말씀이신가요?
2 participants
@ArrayContrains데코레이터를 추가합니다.복합 타입이나 객체 배열의 경우 깊은 비교로 인해 성능 저하를 막을 수 없기 때문에 주의사항을 문서에 추가하였습니다.